docs: Move TopK example code to extending-operators documentation #18372
Conversation
github-actions
bot
added
the
documentation
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Is completely replacing the existing contents of extending-operators.md page an intended change?
gowtham1412-p
force-pushed
the
doc-move-user-defined-plan
branch
from
8eedff8 to
55f9546
Compare
|
@Jefffrey Thank you for catching that! No, completely replacing the content was not intended. I'll update the PR to preserve the existing µWheel documentation and add the TopK example as an additional section. |
|
@Jefffrey I've updated the PR to preserve the existing µWheel documentation. The TopK example is now added as a second example rather than replacing the original content. Please review when you have the convienent time |
| plan: LogicalPlan, | ||
| _config: &dyn OptimizerConfig, | ||
| ) -> Result<Transformed<LogicalPlan>> { | ||
| ) -> Result<Transformed> { |
There was a problem hiding this comment.
I am pretty sure this is an incorrect change --
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
My main concern is the two examples on the page are incongruous. The first example is very minimal and succinct; indeed the syntax examples provided are a tiny snapshot, only showing the rewrite method in isolation of it's original trait OptimizerRule.
Meanwhile the TopK example has also minimal explanation, but all the syntax required in what seems more of a brain dump than a guide. It might be hard for people to follow what is going on since they just see a bunch of code definitions without any explanation of how they relate.
|
please review it once. |
Do you have any thoughts on the comment I left above? |
|
@Jefffrey Thank you for the feedback! I've restructured the TopK example to be more educational and consistent with the µWheel example. The changes include:
Please let me know if this better matches the style you're looking for! |
|
@Jefffrey please review it once,,,if any errors or having any concern let me know it |
|
I will review this when I have time |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
The general layout is looking better, but there's details like syntax being incorrect which I'm quite confused by considering the example code already exists in our codebase 🤔
| ```rust,ignore | ||
| // Converts a uwheel aggregate result to a TableScan with a MemTable as source | ||
| fn agg_to_table_scan(result: f64, schema: SchemaRef) -> Result<LogicalPlan> { | ||
| fn agg_to_table_scan(result: f64, schema: SchemaRef) -> Result { |
There was a problem hiding this comment.
I'm very confused why these changes are being made, especially considering it makes the example invalid?
| ```rust,ignore | ||
| impl OptimizerRule for TopKOptimizerRule { | ||
| fn rewrite( | ||
| &self, | ||
| plan: LogicalPlan, | ||
| _config: &dyn OptimizerConfig, | ||
| ) -> Result<Transformed> { |
There was a problem hiding this comment.
I'm starting to think it's better not to have the ignore as some of this syntax is not correct
3 participants
Which issue does this PR close?
Closes #15774
Rationale for this change
Moving the TopK example code from the test file to user-facing documentation makes it more discoverable for users learning how to create custom operators in DataFusion.
What changes are included in this PR?
extending-operators.mddocumentationAre these changes tested?
Documentation-only change, no functional code changes.
Are there any user-facing changes?
Yes - users can now find the TopK custom operator example in the official documentation instead of having to browse test files.
I've opened PR #18372 to address this issue. Please review at your earliest convenience.